Skip to content

fix(compat): correct injectors to fix issue with compat API on v19 #3595

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 9, 2025

Conversation

rosostolato
Copy link
Contributor

Checklist

Description

Fix compat api methods to work with the updates from #3586.

Code sample

No change to the API usage.

@jamesdaniels
Copy link
Member

Cool. Thanks for this. Unfortunately my compat test app is still broken, so I wasn't able to verify. Was on my list to do before v19 final.

@jamesdaniels jamesdaniels merged commit e567618 into angular:main Jan 9, 2025
19 checks passed
@rosostolato rosostolato deleted the fix-compat-v19 branch January 10, 2025 11:55
@markwhitfeld
Copy link

It looks like this PR inadvertently caused many issues with running the functions of the AngularFirestore service when they are executed outside of an InjectionContext.
All services that are created by an Angular provider are all done so within an injection context.
But, when you new up classes explicitly as is done within the functions of the AngularFirestore service (and possibly other places too) then you rely on the caller to run it within the injection context. This only matters if you use the inject function.

This PR adds the private readonly injector = inject(EnvironmentInjector); line to the AngularFirestoreDocument, AngularFirestoreCollection and AngularFirestoreCollectionGroup classes private fields. All of these classes are newed up explicitly and they therefore fall into a non-determinate injection context. This did not matter before, but now this matters with the use of the inject inside their fields.
This has now introduced the requirement to only create those objects within an injection context, and therefore added this same requirement to the caller of the respective functions of the AngularFirestore service.

Here are some of the reported issues:

As a safe practice, I would recommend not using the inject feature in classes that are not created by an angular provider.
This would force the addition of another constructor parameter for the injector, which would solve this issue.
We could always capture the injector in the provided services and pass the injector through.
This would actually remove the requirement to provide an injection context in order to call functions.

As a general design recommendation, I don't believe we should ever require the user to run within an injection context. The injector, when needed, should rather be captured by the services that are created by the Angular providers, and then provided internally to the code that needs it.

I'll hopefully get a chance to contribute a PR to fix this, but have quite a large migration on my plate at the moment, so I won't get to it now.

@rosslavery
Copy link
Contributor

@markwhitfeld we're being bitten by this currently (for RTDB) -- were you able to work around this by passing an injector in somehow? Or it is too black-boxed by Angularfire internals.

@markwhitfeld
Copy link

My current solution unfortunately is to downgrade to v18. It is too large of a task to update every usage in our codebase to be wrapped in an injection context at this time. Hopefully I will get to creating a PR to fix this is the AngularFire codebase soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants